refactor: decouple JSON extraction rejection from axum by cristi- · Pull Request #348 · s2-streamstore/s2

cristi- · 2026-03-23T13:09:19Z

Summary

Introduces JsonExtractionRejection — an s2-api-owned rejection type replacing direct usage of axum::extract::rejection::JsonRejection in the public API. Consumers import from s2_api::data::extract instead of axum.

Motivation

Consumers of s2-api with the axum feature currently need to import axum::extract::rejection::JsonRejection directly. This couples them to axum's internal rejection types, making it harder to evolve the extraction layer independently.

With this change, consumers only depend on s2_api::data::extract::JsonExtractionRejection, which exposes the same body_text() and status() interface.

Design

All variants are uniform { status: StatusCode, message: Cow<'static, str> } — no special cases
Cow<'static, str> allows zero-alloc construction for static messages (e.g. MissingContentType) while carrying dynamic error strings from the deserializer as Cow::Owned
From<axum::JsonRejection> bridge converts at the boundary — FromRequest still delegates to axum::Json internally
IntoResponse moves the Cow and calls into_owned() — no-op for Cow::Owned, one small alloc for Cow::Borrowed
Enum is #[non_exhaustive] for forward compatibility
Wildcard match arm uses rej.status() (not hardcoded) to handle future axum rejection variants

Allocation impact

Cow reduces the worst case (static-literal construction) but there are still extra clones in the downstream response rendering path (e.g. body_text() returns String, and lite's ServiceError::to_response clones when building ErrorInfo). That's acceptable for this decoupling PR to keep scope tight — it isn't allocation-neutral yet, but the success path is unchanged and the error path costs are honest.

Path	Before	After
Success	zero	zero
MissingContentType construction	zero (axum static)	zero (`Cow::Borrowed`)
Syntax/Data error construction	1 alloc (axum `body_text()`)	1 alloc (`body_text().into()` → `Cow::Owned`)
`IntoResponse` rendering	0 (axum moves internally)	0 for `Cow::Owned` (`into_owned` unwraps), 1 small alloc for `Cow::Borrowed`
`body_text()` caller (e.g. lite)	1 alloc	1 alloc (`into_owned()` → `String`)
`Display` / logging	0	0 (writes from `Cow` directly)

Changes

api/src/data.rs: JsonExtractionRejection enum with Cow<'static, str> messages, From<axum::JsonRejection> bridge, body_text()/status()/IntoResponse/Display/Error impls. FromRequest and OptionalFromRequest for Json<T> now return JsonExtractionRejection. Error classification test verifies 400/422 status code parity with axum.
api/src/v1/stream/extract.rs: AppendRequestRejection wraps JsonExtractionRejection instead of axum::JsonRejection. Imports crate::data::Json instead of axum::Json.
lite/src/handlers/v1/error.rs: Import swap from axum::extract::rejection::JsonRejection to s2_api::data::extract::JsonExtractionRejection.
sdk/src/api.rs: Fix pre-existing dns_error_message_is_clear test failure — install rustls CryptoProvider (required since rustls 0.23+).

Test plan

cargo check --workspace passes
just test — all pass (including previously broken DNS test)
New json_error_classification test: verifies status codes for syntax errors (400) and data errors (422) across 7 input cases
New valid_json_parses_successfully test: verifies happy path

🤖 Generated with Claude Code

Introduce `JsonExtractionRejection` as an s2-api-owned rejection type, replacing direct usage of `axum::extract::rejection::JsonRejection` in the public API. This gives us control over the error boundary without coupling consumers to axum's internal rejection types. The `FromRequest` impl still delegates to `axum::Json` internally — the only change is that the rejection is converted at the boundary via `From<axum::JsonRejection>`. Zero behavioral change, zero performance impact on the success path. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

greptile-apps · 2026-03-23T13:12:02Z

Greptile Summary

This PR introduces JsonExtractionRejection — an s2-api-owned rejection type that replaces direct usage of axum::extract::rejection::JsonRejection in the public API. Consumers now import from s2_api::data::extract instead of coupling to axum's internal types, fitting naturally into the existing layered error hierarchy (AppendRequestRejection → ServiceError).

JsonExtractionRejection uses Cow<'static, str> for zero-alloc construction of static messages (e.g. MissingContentType) while carrying dynamic serde errors as Cow::Owned.
From<axum::JsonRejection> provides a clean conversion bridge; the #[non_exhaustive] attribute and wildcard arm future-proof the type.
body_text() now returns &str rather than String (the PR description says String, but the implementation is better — no allocation on the read path).
The previous concern about classify_json_error being pub is fully resolved: it is now a private function inside the #[cfg(test)] module.
The unrelated dns_error_message_is_clear test fix installs the rustls CryptoProvider correctly, discarding duplicate-install errors with let _ = to handle concurrent test runners.
One minor style nit: the inner match message { Cow::Borrowed(s) => ..., Cow::Owned(s) => ... } in IntoResponse uses two syntactically identical arms because Cow::Borrowed and Cow::Owned bind different inner types; message.into_owned() would be a cleaner spelling.

Confidence Score: 5/5

Safe to merge — clean decoupling refactor with no behavioural changes on the success path and preserved status-code semantics on the error path.
All four files are either mechanical import swaps or a well-tested new type. The previous classify_json_error visibility concern is resolved. Error status codes (400/422/415) are preserved through the From bridge and validated by the new json_error_classification test. The SDK fix is a pre-existing test reliability issue unrelated to the main change. The only remaining item is a non-blocking style nit on the match message inner arms.
No files require special attention.

Important Files Changed

Filename	Overview
api/src/data.rs	Core change: introduces `JsonExtractionRejection` with `Cow<'static, str>` messages, `From<axum::JsonRejection>` bridge, and updated `FromRequest`/`OptionalFromRequest` impls. Well-structured with `#[non_exhaustive]` for forward-compat and good test coverage. Previous concern about `classify_json_error` visibility is resolved — now a private fn inside `#[cfg(test)]`.
api/src/v1/stream/extract.rs	Mechanical import swap: `JsonRejection` from axum replaced with `JsonExtractionRejection` from `crate::data::extract`, and `axum::Json` replaced with `crate::data::Json`. No logic changes.
lite/src/handlers/v1/error.rs	Import swap only: `axum::extract::rejection::JsonRejection` replaced with `s2_api::data::extract::JsonExtractionRejection`. The `to_response` call site for `JsonRejection` now uses `body_text()` which returns `&str` instead of `String` — compatible with `impl Into<String>` in `standard()`.
sdk/src/api.rs	Single-line fix for pre-existing `dns_error_message_is_clear` test failure: installs `aws_lc_rs` as rustls default crypto provider. Uses `let _ =` to silently discard errors from concurrent test runs that try to install the provider multiple times.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[HTTP Request] --> B[axum::Json FromRequest]
    B -->|Success| C[Json value]
    B -->|Failure| D[axum::JsonRejection]

    D -->|JsonDataError| E1[JsonExtractionRejection::DataError\nstatus: 422, message: Cow::Owned]
    D -->|JsonSyntaxError| E2[JsonExtractionRejection::SyntaxError\nstatus: 400, message: Cow::Owned]
    D -->|MissingJsonContentType| E3[JsonExtractionRejection::MissingContentType\nstatus: 415, message: Cow::Borrowed static]
    D -->|Other| E4[JsonExtractionRejection::Other\nstatus: from rej, message: Cow::Owned]

    E1 & E2 & E3 & E4 --> F{Caller}
    F -->|AppendRequestRejection| G[IntoResponse\ndirect status from rejection]
    F -->|ServiceError| H[to_response\nstatus from ErrorCode::BadJson]

    G --> I[HTTP Response]
    H --> I

Prompt To Fix All With AI

This is a comment left during a code review.
Path: api/src/data.rs
Line: 162-168

Comment:
**Redundant inner `match` on `Cow` variant**

The two arms of the inner `match message { ... }` body are syntactically identical. While the arms are correct (binding different inner types, `&'static str` vs `String`, both of which implement `IntoResponse`), the pattern reads as though the two cases are handled differently.

Since `SyntaxError`, `DataError`, and `Other` are always constructed from dynamic error messages (via `.into()` on a `String`), the `Cow::Borrowed` arm is unreachable in practice today. Simplifying to `message.into_owned()` makes the intent clearer and removes the dead branch:

```suggestion
                Self::SyntaxError { message, .. }
                | Self::DataError { message, .. }
                | Self::Other { message, .. } => (status, message.into_owned()).into_response(),
```

`into_owned()` is a no-op for `Cow::Owned` and adds one alloc for `Cow::Borrowed`, which is acceptable in an error path.

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (3): Last reviewed commit: "perf: zero-alloc MissingContentType and ..." | Re-trigger Greptile}

api/src/data.rs

- Move `classify_json_error` into `#[cfg(test)]` module to avoid leaking axum-coupled helper through the public API (greptile review feedback) - Install rustls CryptoProvider in `dns_error_message_is_clear` test to fix pre-existing panic with rustls 0.23+ Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

cristi- · 2026-03-23T13:15:45Z

@greptileai

All variants now store `{ status, message }` uniformly. The message is captured once in the `From<JsonRejection>` bridge and reused directly in `body_text()`, `Display`, and `IntoResponse` — no re-allocation. `IntoResponse` moves the String instead of cloning. `body_text()` does one clone (matches axum's existing behavior where consumers call it). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Static messages (MissingContentType) are now zero-alloc. Dynamic messages from axum's body_text() are stored as Cow::Owned. Display writes directly from the Cow — no intermediate allocation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

into_response() now destructures and moves the Cow, calling into_owned() which is a no-op for Cow::Owned (just unwraps). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

cristi- · 2026-03-23T13:36:42Z

@greptileai

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

- MissingContentType is now a unit variant with a static message const — zero heap allocation on construction, display, and response rendering - body_text() returns &str instead of String — no clone needed - IntoResponse matches on Cow: Borrowed passes &str directly, Owned moves the String — no intermediate allocation in either case Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

cristi- · 2026-03-23T13:43:03Z

@greptileai

greptile-apps · 2026-03-23T13:46:23Z

api/src/data.rs

+            match self {
+                Self::SyntaxError { message, .. }
+                | Self::DataError { message, .. }
+                | Self::Other { message, .. } => match message {
+                    Cow::Borrowed(s) => (status, s).into_response(),
+                    Cow::Owned(s) => (status, s).into_response(),
+                },


Redundant inner match on Cow variant

The two arms of the inner match message { ... } body are syntactically identical. While the arms are correct (binding different inner types, &'static str vs String, both of which implement IntoResponse), the pattern reads as though the two cases are handled differently.

Since SyntaxError, DataError, and Other are always constructed from dynamic error messages (via .into() on a String), the Cow::Borrowed arm is unreachable in practice today. Simplifying to message.into_owned() makes the intent clearer and removes the dead branch:

Suggested change

match self {

Self::SyntaxError { message, .. }

| Self::DataError { message, .. }

| Self::Other { message, .. } => match message {

Cow::Borrowed(s) => (status, s).into_response(),

Cow::Owned(s) => (status, s).into_response(),

},

Self::SyntaxError { message, .. }

| Self::DataError { message, .. }

| Self::Other { message, .. } => (status, message.into_owned()).into_response(),

into_owned() is a no-op for Cow::Owned and adds one alloc for Cow::Borrowed, which is acceptable in an error path.

Prompt To Fix With AI

This is a comment left during a code review. Path: api/src/data.rs Line: 162-168 Comment: **Redundant inner `match` on `Cow` variant** The two arms of the inner `match message { ... }` body are syntactically identical. While the arms are correct (binding different inner types, `&'static str` vs `String`, both of which implement `IntoResponse`), the pattern reads as though the two cases are handled differently. Since `SyntaxError`, `DataError`, and `Other` are always constructed from dynamic error messages (via `.into()` on a `String`), the `Cow::Borrowed` arm is unreachable in practice today. Simplifying to `message.into_owned()` makes the intent clearer and removes the dead branch: ```suggestion Self::SyntaxError { message, .. } | Self::DataError { message, .. } | Self::Other { message, .. } => (status, message.into_owned()).into_response(), ``` `into_owned()` is a no-op for `Cow::Owned` and adds one alloc for `Cow::Borrowed`, which is acceptable in an error path. How can I resolve this? If you propose a fix, please make it concise.

greptile-apps bot reviewed Mar 23, 2026

View reviewed changes

api/src/data.rs Outdated Show resolved Hide resolved

cristi- and others added 4 commits March 23, 2026 15:25

chore: trim doc comments

8db6795

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

perf: move Cow in IntoResponse instead of to_string()

fc1fd56

into_response() now destructures and moves the Cow, calling into_owned() which is a no-op for Cow::Owned (just unwraps). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

cristi- and others added 2 commits March 23, 2026 15:36

chore: mark From<JsonRejection> bridge as temporary

1226648

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

greptile-apps bot reviewed Mar 23, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: decouple JSON extraction rejection from axum#348

refactor: decouple JSON extraction rejection from axum#348
cristi- wants to merge 8 commits intomainfrom
cc/decouple-json-rejection

cristi- commented Mar 23, 2026 •

edited

Loading

Uh oh!

greptile-apps bot commented Mar 23, 2026 •

edited

Loading

Uh oh!

Uh oh!

cristi- commented Mar 23, 2026

Uh oh!

cristi- commented Mar 23, 2026

Uh oh!

cristi- commented Mar 23, 2026

Uh oh!

greptile-apps bot Mar 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

cristi- commented Mar 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Motivation

Design

Allocation impact

Changes

Test plan

Uh oh!

greptile-apps bot commented Mar 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Flowchart

Uh oh!

Uh oh!

cristi- commented Mar 23, 2026

Uh oh!

cristi- commented Mar 23, 2026

Uh oh!

cristi- commented Mar 23, 2026

Uh oh!

greptile-apps bot Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

cristi- commented Mar 23, 2026 •

edited

Loading

greptile-apps bot commented Mar 23, 2026 •

edited

Loading